-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend type support on JWT claims #451
Conversation
* | ||
* @param claims Map of claim objects to be validated | ||
*/ | ||
private static boolean validateClaims(Map<String, ?> claims) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting code. I wonder if there's a library for this somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked a fellow programmer friends who are more experience than me on Java before coming up with this and they didn't know of any.
I ultimately got inspired by the logic used on java-jwt though I wanted to provide a more robust support than that implementation.
Open to any suggestions you may have on how to improve this code!
oauth2_http/javatests/com/google/auth/oauth2/JwtClaimsTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like a SWE on the oauth team to review this.
oauth2_http/javatests/com/google/auth/oauth2/JwtClaimsTest.java
Outdated
Show resolved
Hide resolved
Additional JWT claims now support more types other than String
* @param claims Map of claim objects to be validated | ||
*/ | ||
private static boolean validateClaims(Map<String, ?> claims) { | ||
if (!validateKeys(claims)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really required here given that the key type is explicitly defined as String?
* @param value object to be evaluated. | ||
*/ | ||
private static final boolean isSupportedValue(Object value) { | ||
Class clazz = value.getClass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this and pass into contains directly? since its trivial and not reused
@@ -68,10 +73,10 @@ | |||
* | |||
* @return additional claims | |||
*/ | |||
abstract Map<String, String> getAdditionalClaims(); | |||
abstract Map<String, ?> getAdditionalClaims(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a breaking change. It should be possible to keep the old one marked as Deprecated and calling this new signature internally
Closing as outdated, feel free to reopen if actual again |
Additional JWT claims now support more types other than String
Fixes #439